Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update hooks example to handle redirection #1195

Merged
merged 12 commits into from
Apr 13, 2022

Conversation

caseploeg
Copy link

Previously redirection information was lost by the postparsing hooks add_whitespace_hook() and downcase_hook(). This was fine for this simple shell, but sets a bad example for more complicated use cases. I made this mistake in my own application where I set up a hook to augment a Statement's arguments, but lost redirection information by accident. A very sticky bug to try and figure out why piping suddenly no longer works.

To fix this, when parsing the new Statement object, include the post_command attribute from the original Statement. I also made some slight organizational changes that should make it more obvious what's happening in the example.

I also add a postcommand hook that hopefully provides some more intuition on the hook lifecycle by updating the prompt with the raw contents of the command last entered. I have this feature locked behind the debug option. I imagine this could help someone figure out the consequences of registering new hooks.

related to #1000

Case Ploeg added 2 commits January 27, 2022 16:26
Previously redirection information was lost by the postparsing hooks
add_whitespace_hook() and downcase_hook(). This was fine for this simple
shell, but sets a bad example for more complicated use cases.

To fix this, when parsing the new Statement object, include the
`post_command` attribute from the original Statement.
Not necessary for the sake of the example, but might help some curious
individuals get a better feel for how the system works.
@caseploeg caseploeg requested a review from tleonhardt as a code owner January 27, 2022 22:03
anselor
anselor previously approved these changes Feb 11, 2022
Copy link
Contributor

@anselor anselor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking things down in the example to improve clarity looks good.

@codecov
Copy link

codecov bot commented Feb 12, 2022

Codecov Report

Merging #1195 (0d2e4b8) into master (036f185) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1195   +/-   ##
=======================================
  Coverage   98.52%   98.52%           
=======================================
  Files          22       22           
  Lines        5695     5695           
=======================================
  Hits         5611     5611           
  Misses         84       84           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 036f185...0d2e4b8. Read the comment docs.

@anselor
Copy link
Contributor

anselor commented Feb 13, 2022

@tleonhardt The lint check is failing on unrelated files. I think something in black must have changed on us?

@tleonhardt
Copy link
Member

@anselor I merged in a fix for the black issues in unrelated files

@tleonhardt
Copy link
Member

@caseploeg Thank you for the PR! Please run flake8 to fix linting issues and black to fix formatting ones and update the PR.

@kmvanbrunt kmvanbrunt merged commit 53fc617 into python-cmd2:master Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants